-
Notifications
You must be signed in to change notification settings - Fork 521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #1547, parts of #169, #5344, #5365, #5411: Add data layer support for (multiple) classrooms & topic dependencies, and prepare for #4885 #5398
Conversation
This doesn't actually finish the new data definitions or properly integrate them, nor is it updating any local development assets.
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Also, prep for other proto dependencies that will be needed by the asset download script.
This functionality hasn't been used in years, and it's unnecessary. Plus, some lint fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed all changes.
app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/PrimeTopicAssetsController.kt
Show resolved
Hide resolved
data/src/main/java/org/oppia/android/data/backends/gae/model/BUILD.bazel
Show resolved
Hide resolved
data/src/test/java/org/oppia/android/data/backends/gae/RemoteAuthNetworkInterceptorTest.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Show resolved
Hide resolved
Conflicts: third_party/BUILD.bazel utility/src/main/java/org/oppia/android/util/caching/BUILD.bazel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed 2 changed files after merge (due to conflicts from develop changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed latest changes.
This ensures the fixes explained in bazelbuild/rules_proto#117 and bazelbuild/bazel#16192 are included to fix CI breakages on this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed latest changes (bumping oppia-proto-api version so that CI doesn't try to pull a non-existent version of zlib).
This pulls in the GCS endpoint from #5398 as part of a broader effort of splitting it up.
Ah excellent. CI is now passing! @adhiamboperes PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @BenHenning! The cleanup looks spiff and I have no concerns with the new additions.
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Thanks @adhiamboperes! I really appreciate the review. :) Updating to latest develop & enabling auto-merge. |
Also @theMr17 I forgot to ask you to review this, so I suggest taking a look at it post-merge and let me know if there are any additions needed for multiple classrooms that won't already be included in your project. |
Explanation
Fixes #1547
Fixes part of #169
Fixes part of #5344
Fixes part of #5365
Fixes part of #5411
The main purpose of this PR is to introduce support for multiple classrooms in the data layer of the app (with minimal domain integration to avoid the change extending beyond the lesson structures). However, the PR is also doing a few more things including preparing the Android codebase for introducing an asset download script (#4885) and other peripheral cleanups of code (rather than updating it) that was noticed along the way.
Preparing for multiple classrooms
This addresses part of #5365.
#5344 is introducing support for multiple classrooms in the app. To help prepare for those changes, this PR introduces the necessary data structure and domain loading changes to load a new proto structure:
Rather than just a flat topic list. Some important details to note:
topic_prerequisites
value being loaded from classrooms. This will also extend to production assets since the asset download script from Fix #5411: Introduce asset download script [Blocked: #5416] #4885 is also being updated to include support for this multiple classrooms structure to address the remainder of [Feature Request]: Make Changes to Download Script #5365.classrooms
,id
, andtopic_prerequisites
(includingtopic_ids
) from the protos above. These other fields are expected to have supported added as part of [Feature Request]: Introduce support for multiple classrooms (GSoC'24 4.2) #5344.TopicListController
. These largely shouldn't have a major impact outside of developer assets. These changes were made to ensure non-specificity to the previous lesson classroom. In general, all of this should eventually be removed in favor of loading colors from lesson assets, but that will need to wait until the JSON pipeline is completely removed.Asset priming removal
This addresses part of #169.
PrimeTopicAssetsController
and its implementation were responsible for hackily pre-loading all lesson images and audio to be on-device to enable offline support. This was the first attempt at offline support early in the app's development, but it had a few significant drawbacks:This approach was abandoned after the earliest alpha releases for an asset download script (which is now being migrated over to this codebase per #5411.
This removal unfortunately required removing a module that was referenced in a lot of tests throughout the codebase. While the removal itself was fairly simple, it does affect a lot of files.
Other areas changed (but unaffected by tests since these flows didn't have automated tests):
SplashActivityPresenter
for enabling the downloader to start and block the UI using a dialog box while the downloading occurred.GlideImageLoader
for removing support for loading locally cached images (only through this flow; the flow we use for the asset download script uses a different annotation and is still supported).As alluded to above, two annotations were removed as part of this cleanup:
CacheAssetsLocally
: a boolean indicating whether the prime download flow should be enabled.TopicListToCache
: this specified the list of topics to pre-download if the flow was enabled.GAE structure cleanup & preparing for asset script
Preparing for #4885 included a few other changes, some of which were nice-to-have:
java_proto
versions for many of the app's proto structures (since the download script runs in the JVM and doesn't use the javalite proto environment).RemoteAuthNetworkInterceptorTest
was updated to use a different example service sinceTopicService
has been removed. The services for platform parameters and user feedback are being kept.Note that the new Java proto targets & oppia-proto-api targets aren't being used yet, but they will be in future PRs. This just provides the basis of support for the asset download script while also helping to split up the code to review across multiple PRs.
Essential Checklist
For UI-specific PRs only
This is essentially only data infrastructural except for a couple of points: